-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support bberg lookups #37
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work! Please address/look at my feedback before merging.
bberg/src/circuit_builder.rs
Outdated
return false; | ||
}}", | ||
permutation_name = permutation_name | ||
lookup_name = lookup_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I would prefer two different identifiers, unless this is Rust standard practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be replaced with the single identifier, as it is just for string templating - will edit
bberg/src/lookup_builder.rs
Outdated
} | ||
|
||
#[derive(Debug)] | ||
/// PermSide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy-paste leftover? Probably should be "LookupSide"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually good point here- they are more or less the same i can unify the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually i probably wont, even though it is repeated it is much more readable when they are defined beside each other
) -> Vec<Lookup>; | ||
} | ||
|
||
impl LookupBuilder for BBFiles { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this function and subsequent ones, replace variables like "perm", "perm_settings" by "lookup", "lookup_settings".
bberg/src/lookup_builder.rs
Outdated
* | ||
* @details To create your own lookup: | ||
* 1) Create a copy of this class and rename it | ||
* 2) Update all the values with the ones needed for your permutation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 2) Update all the values with the ones needed for your permutation | |
* 2) Update all the values with the ones needed for your lookup |
static constexpr size_t READ_TERM_TYPES[READ_TERMS] = {read_term_types}; | ||
|
||
/** | ||
* @brief They type of WRITE_TERM used for each write index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @brief They type of WRITE_TERM used for each write index | |
* @brief The type of WRITE_TERM used for each write index |
bberg/src/lookup_builder.rs
Outdated
}}") | ||
} | ||
|
||
fn get_perm_side<F: FieldElement>(def: &SelectedExpressions<AlgebraicExpression<F>>) -> LookupSide { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename function to "get_lookup_side"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shall unify these
{compute_inverse_exists} | ||
|
||
/** | ||
* @brief Get all the entities for the lookup when need to update them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @brief Get all the entities for the lookup when need to update them | |
* @brief Get all the entities for the lookup when we need to update them |
see AztecProtocol/aztec-packages#3880 for implementation example
Lookups allow us to assert that values in a set of columns equal some values in another set of columns.
This approach uses the log derivative method to perform lookups, where the lookup relation requires four extra columns ( as well as the read and write columns ) to function.
The selector columns will need to be defined manually - as in the
ToyAvm
example.The inverse column and the counts columns are automatically included by the code gen. ( this means there are two extra columns per lookup ( which is a little bit inefficient; but will do for now ).
The name of the inverse column is decided by the attribute tag
#[attribute]
above the lookup - which is required.The name of the counts column is
${attribute}_counts
.Note: that in the circuit builder you are currently required to set the counts column value manually.
syntax: